Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Travel blog #49

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Travel blog #49

wants to merge 7 commits into from

Conversation

NelchenB
Copy link

I wonder what you think about my project. It's not responsible yet but I'm woring on it.

Copy link

@karinnordkvist karinnordkvist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really good job on your webside! I like the pictures a lot, and the code has a really good semantic structure with correct hierarchy.

I have written some tips & tricks, mostly in the css. 😊 The main thing I would like to say is to try to use more of scaleable units instead of px. Px are mostly used for margins and font-sizes, but for wrappers and boxes, as well as for images, it's better to use % or vw/vh. That way your code becomes automatically responsive. 👍🏻

Keep up the good work! 🌈

@@ -0,0 +1,196 @@
@import url(https://fonts.googleapis.com/css?family=Reenie+Beanie);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi there! My name is Karin and I will take a review your code today 😊

Nice with a custom font! 😊👍

@import url(https://fonts.googleapis.com/css?family=Reenie+Beanie);

body{
background:rgb(238, 232, 170)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using rgb-values are perfectly fine, but it's more common to use hex-values, for example #ffffff = white. There is also an option to use rgba(xx, xx, xx, AA), where AA stands for alpha (opacity). And don't forget about the semi-colons at the end! 😊😊

}


.paragraph {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to target the p-tag only, the paragraph, you don't need to use a class name for it. :) All you need is p, without a dot or anything else, like so:

p {
....
}

display: flex;
align-items: center;
justify-content: space-between;
font-family: "Reenie Beanie";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want "Reenie Beanie" applied to the entire page, you could put it on the body-tag, and specify if you want a different font wherever you want it. 😊

Like this:

body {
    font-family: "Reenie Beanie";
}

.some-other-class {
    font-family: "Some other font";
}

Comment on lines +71 to +74
#galerie {
box-sizing: border-box;
background-size: 58px 58px;
background-position: 0px 2px, 4px 35px, 29px 31px, 34px 6px;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what these three rows are doing, but I think another way going about this would be to use flexbox, and having the box model in mind. 😄 I would also recommend using a class instead of an id for this. 👍🏻 For example, you could put the following on this tag:

.galerie {
    // will set the wrapper-box to full width
    width: 100%;

     // These three rows sets the display to flex, and place all content in the center
    display: flex;
    align-items: center; 
    justify-content: center; 

    // When you've assigned a width to all divs in this wrapper, you'll have to use flex to make 
    // the fourth box move down to the second row.
    flex-wrap: wrap; 
}

....


#galerie figcaption {
color: rgb(96, 96, 84);
font: cursive 10px/150%;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make the font cursive and changing size/line height, you'll have to write like this:

font-style: italic;
font-size: 10px;
line-height: 150%;

background-position: center;
background-repeat: no-repeat;
background-size: cover;
background-attachment: fixed;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! Looks super nice! 🌟

Comment on lines +141 to +157
.parallaxwir {
background-image: url("images/DavosEisstadion.jpg");
min-height: 60vh;
background-position: center;
background-repeat: no-repeat;
background-size: cover;
background-attachment: fixed;
}

.parallaxplan {
background-image: url("images/LaaxSkipiste.jpg");
min-height: 60vh;
background-position: center;
background-repeat: no-repeat;
background-size: cover;
background-attachment: fixed;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want, you could separate the background-image source from the other settings into two classes, to avoid having to repeat the same settings every time. 😊

For example:
<div class="parallaxwir"></div>
could instead be:
<div class="parallax wir"></div>

and the styling would look like this:

.parallax {
        min-height: 60vh;
	background-position: center;
	background-repeat: no-repeat;
	background-size: cover;
        background-attachment: fixed;
}

.wir {
	background-image: url("images/LaaxSkipiste.jpg");
}

By doing like this, you would only have to add more source-classes if you want the image to be different. 👌

For example:
<div class="parallax plan"></div>
+

.plan {
	background-image: url("images/other-image.jpg");
}

<link rel="stylesheet" type="text/css" href="ontour_20211120.css"/>
</head>

<body>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good usage of semantic HTML-tags! 🌟

Comment on lines +26 to +29
<main>
<article>
<h2 id="wir">Wir fünf auf Tour</h2>
<p class="paragraph">Ein bisschen verrückt, ein bisschen chaotisch und ein bisschen bünzli.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should try to add some indentation to your code, to make it easier to read. 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants